Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade to python 3, flask 2, and other dependencies #84

Merged
merged 1 commit into from
Jun 7, 2022

Conversation

alastair
Copy link
Collaborator

I'm not sure why redis had a guard on it to not install versions greater than v4. @amCap1712 do you know why? I didn't add a specific reason in #45

@alastair alastair requested a review from amCap1712 April 11, 2022 10:26
@github-actions

This comment has been minimized.

requirements.txt Outdated Show resolved Hide resolved
requests>=2.23.0
SQLAlchemy>=1.3.16
requests>=2.27.1
SQLAlchemy>=1.3.16,<2.0
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

big API changes in sqlalchemy2, so I decided to pin this here for now until we can be sure that mbdata works with it

@@ -12,5 +12,5 @@
packages=find_packages(),
use_scm_version=True,
setup_requires=['setuptools_scm'],
install_requires=open("requirements.txt").read().split(),
install_requires=open("requirements.txt").read().splitlines(),
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this fixes some of the issues that we had in requirements file (it was splitting on whitespace, not newline). We could also consider removing comments:

[line for line in open("requirements.txt").read().splitlines() if line and not line.startswith('#')]

Still doesn't remove inline comments. I wonder if a specific requirements parser would be a better idea?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fix with a comment in requirements.txt is good enough IMO.

That said, I guess its fine to use a special parser for this or moving this other way around as that answer suggests etc. Maybe open a ticket for later improvement.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

of course, perhaps the better solution would be to switch to poetry/pyproject.toml instead, which should be able to reuse the same dependency list...

@github-actions

This comment has been minimized.

@amCap1712
Copy link
Member

amCap1712 commented May 24, 2022

I don't know why that guard exists but if I were to guess, redis-4.0 (redis 4 wasn't released until much later that year but the documentation of redis 3.5.3 specified that 4.0 wouldn't support py2.7) does not support py2.7 but at time of #45 BU still supported py2.7.

Copy link
Member

@amCap1712 amCap1712 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, Thanks!

But we should probably test at least 1 downstream app before merging and releasing.

@alastair alastair force-pushed the version-upgrades branch from 20ec67c to bcc913c Compare May 31, 2022 11:15
@github-actions

This comment has been minimized.

@amCap1712 amCap1712 merged commit 31cbfcb into master Jun 7, 2022
@amCap1712 amCap1712 deleted the version-upgrades branch June 7, 2022 10:08
@github-actions
Copy link

github-actions bot commented Jun 7, 2022

Unit Test Results

  1 files  ±0    1 suites  ±0   40s ⏱️ ±0s
81 tests ±0  81 ✔️ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit 31cbfcb. ± Comparison against base commit 31cbfcb.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants